-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds attribute mutation if suspended. #30
Conversation
Also refactored to newer extenders.
function (Dispatcher $events) { | ||
$events->subscribe(Listener\AddUserSuspendAttributes::class); | ||
$events->subscribe(Listener\RevokeAccessFromSuspendedUsers::class); | ||
(new Extend\Event()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need / want to create whole classes for these new listeners? They seem small / simple enough that they could just be closures, and don't need any DI
Consider this to be a comment / question rather than a "changes request". Both options have merits, although from a modularization perspective I think I prefer separate classes as done here. Just raising this as a potential point of discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally do like using invokable classes instead, I'm not a big fan of having logic code in the extend.php file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not tested locally, but code makes sense
This does mean the suspended user won't also be able to edit their notification preferences (especially email notifications). |
Should suspended users even get email notifications? |
I'd say yes, especially if you want to notify them of their suspension/unsuspension. Btw, what I'm referring to above especially is that suspended users not being able to disable email notifications would be an issue. |
Hi, waiting for this fix. I want try but seems the fix should be updated for Beta 15 as is not seems to be working. |
In that case, perhaps we could check $user->getDirty(), and if it contains anything other than notification preference changes (or user preference changes at all for that matter, I think we still store all of them together in json rn), only then throw the error? |
Yea, and for extensions, how about having an extender to include (or exclude) attributes ? Something like this perhaps ? (new SuspendExtend\User)
->exclude('attribute_name') |
Not a bad idea, but let's wait until L8 and flarum/framework#2544 so we can add it with integration testing. I think that can also probably be done in a separate PR? |
Would be better yes. |
Thought about this a bit more. If we add a policy for edit on User, we can restrict editing stuff like bio, avatar, masquerade fields, nickname, etc without infringing on preferences, mark as read, and credentials. I think that's the substantially better option here. |
Fixes flarum/core#2136.
This PR adds two new listeners and refactors some code to new extenders.
I'm aware this is not what has been discussed in the issue. But this is a very sane solution to prevent abuse across other extensions.